-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Send saml logout response even when validation errors happen #14676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for the PR, @1livv. This is going to take a little bit more work to maintain backward compatibility and also ensure that the APIs progress naturally. I'll get back to you soon with recommendations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @1livv! This is very helpful. I've left my feedback inline.
...-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java
Outdated
Show resolved
Hide resolved
...-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java
Outdated
Show resolved
Hide resolved
...-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java
Outdated
Show resolved
Hide resolved
...work/security/saml2/provider/service/web/authentication/logout/Saml2LogoutRequestFilter.java
Show resolved
Hide resolved
...k/security/saml2/provider/service/web/authentication/logout/Saml2LogoutResponseResolver.java
Outdated
Show resolved
Hide resolved
...ngframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java
Outdated
Show resolved
Hide resolved
...-service-provider/src/main/java/org/springframework/security/saml2/core/Saml2ErrorCodes.java
Outdated
Show resolved
Hide resolved
...work/security/saml2/provider/service/web/authentication/logout/Saml2LogoutRequestFilter.java
Show resolved
Hide resolved
...ngframework/security/config/annotation/web/configurers/saml2/Saml2LogoutConfigurerTests.java
Show resolved
Hide resolved
Hi, @1livv, are you able to make the requested changes? |
Hey @jzheaux this slipped off my radar, i'll take a look in the next days. |
@1livv I'd be willing to take care of the change requests if you are ok with this |
Yes, that would be great thank you @jan-knoblich !! |
@1livv Could you Sync your repository with the main one? Otherwise I will have to create a new one. There are some changes that impact the changes in this PR |
@1livv and @jan-knoblich, I've rebased the PR, thanks for your patience. Would either of you like to apply the listed changes? If I don't hear from you one way or the other by next week, I'll go ahead and provide a polish commit myself. |
Signed-off-by: Liviu Gheorghe <[email protected]>
Signed-off-by: Liviu Gheorghe <[email protected]>
Signed-off-by: Liviu Gheorghe <[email protected]>
Signed-off-by: Liviu Gheorghe <[email protected]>
Thanks, @1livv! This is now merged into |
solves #14635